Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Administrative area bug fix #19

Merged
merged 2 commits into from
Jul 26, 2015
Merged

Conversation

brendanebers
Copy link
Contributor

Changed administrative_area from a class attribute to an instance attribute. This fixes an issue where multiple search results would share the same list of administrative areas.

To see this bug, open up a python shell and search for two different places.

from geolocation.google_maps import GoogleMaps
maps = GoogleMaps('AIzaSyDNvdrZ_HEtfsuPYHV9UvZGc41BSFBolOM')
houston = maps.search('Houston, TX').first()
rio = maps.search('Rio de Janeiro').first()
houston.administrative_area is rio.administrative_area
True

…ribute. This fixes an issue where multiple search results would share the same list of administrative areas.
slawek87 added a commit that referenced this pull request Jul 26, 2015
Administrative area bug fix
@slawek87 slawek87 merged commit 9b0995d into slawek87:master Jul 26, 2015
@slawek87
Copy link
Owner

Good job! Maybe you have idea how to improve tests? Problems is that google is improving and updating geo data.. and tests have old data.

@brendanebers
Copy link
Contributor Author

Ideally tests shouldn't call google's services, because the data is subject to change. You might want to look into mocking the responses for different calls. For a brief intro to mock: http://www.toptal.com/python/an-introduction-to-mocking-in-python

@slawek87
Copy link
Owner

ok! Next good idea! :) I'll implement that solution in this week :) If you have some other ideas let me know! Maybe we can together create something really good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants